Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

test:parameters of fs read and readsync #17910

Closed
wants to merge 4 commits into from
Closed

test:parameters of fs read and readsync #17910

wants to merge 4 commits into from

Conversation

bacriom
Copy link
Contributor

@bacriom bacriom commented Dec 29, 2017

added test-fs-read-sync-constructor-errors and test-fs-read-constructor-errors
tests added to extend the code coverage testing of the project.
ref: pull closed #17875

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Dec 29, 2017
lpinca
lpinca reviewed Dec 29, 2017
View reviewed changes
test/parallel/test-fs-read-parameters-errors.js Outdated
const fs = require('fs');
const filepath = fixtures.path('x.txt');
const fd = fs.openSync(filepath, 'r');
const expected = Buffer.from('xyz\n');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this buffer needed? It seems that only its length is used below.
I would replace it with const length = 4;.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have changed it

test/parallel/test-fs-read-sync-parameters-errors.js Outdated

[true, null, undefined, () => {}, {}].forEach((value) => {
common.expectsError(() => {
fs.read(value,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be fs.readSync.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, sorry, I have fixed it

BridgeAR
BridgeAR approved these changes Dec 29, 2017
View reviewed changes
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM in general but adding a check for the message in all cases would be nice.

test/parallel/test-fs-read-parameters-errors.js Outdated
length,
0,
common.mustNotCall());
}, { code: 'ERR_INVALID_ARG_TYPE', type: TypeError });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to test for the error message as well to make sure the right error is triggered.

@bacriom
test:parameters of fs read and readsync
ee7e0c6 
added test-fs-read-sync-constructor-errors and test-fs-read-constructor-errors

tests added to extend the code coverage testing of the project
@bacriom
test:parameters of fs read and readsync
7af2784 
added test-fs-read-sync-constructor-errors and test-fs-read-constructor-errors

tests added to extend the code coverage testing of the project
@bacriom
test:parameters of fs read and readsync
14e1660 
added test-fs-read-sync-constructor-errors and test-fs-read-constructor-errors

tests added to extend the code coverage testing of the project
@BridgeAR
Copy link
Member

CI https://ci.nodejs.org/job/node-test-pull-request/12349/

@BridgeAR BridgeAR added fast-track PRs that do not need to wait for 48 hours to land. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Dec 29, 2017
maclover7
maclover7 reviewed Jan 2, 2018
View reviewed changes
Copy link
Contributor

@maclover7 maclover7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you be able to move these test cases to test/parallel/test-fs-read-type.js? It looks like there are already some type checking tests for fs.read and fs.readSync there.

@tniessen tniessen removed the fast-track PRs that do not need to wait for 48 hours to land. label Jan 7, 2018
@joyeecheung
Copy link
Member

Ping @bacriom , do you have the time to address #17910 (review) ?

@joyeecheung joyeecheung removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 17, 2018
@bacriom
Copy link
Contributor Author

bacriom commented Jan 17, 2018

sorry for the answer too late, ok, I'll move my tests cases to test/parallel/test-fs-read-type.js

@joyeecheung
Copy link
Member

@bacriom Thanks. CI: https://ci.nodejs.org/job/node-test-pull-request/12587/

@targos
Copy link
Member

targos commented Jan 20, 2018

Test failures are not related

@targos targos added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 20, 2018
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 21, 2018
@bacriom @BridgeAR
test: check fs.read and fs.readsync input
080a72c 
Added tests to extend the code coverage.

PR-URL: nodejs#17910
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Weijia Wang <starkwang@126.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@BridgeAR
Copy link
Member

Landed in 080a72c

@bacriom congratulations on your first commit to Node.js! 🎉

@BridgeAR BridgeAR closed this Jan 21, 2018
@MylesBorins
Copy link
Contributor

This does not land cleanly on v9.x, would a collaborator be willing to backport?

MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@bacriom @MayaLekova
test: check fs.read and fs.readsync input
cba0fc8 
Added tests to extend the code coverage.

PR-URL: nodejs#17910
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Weijia Wang <starkwang@126.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maclover7 maclover7 maclover7 left review comments

@starkwang starkwang starkwang approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@cjihrig cjihrig cjihrig approved these changes

@targos targos targos approved these changes

@lpinca lpinca lpinca approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.